Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add propertyNamesSpliterator #1369

Merged
merged 3 commits into from
Dec 28, 2024
Merged

Conversation

pjfanning
Copy link
Member

will likely break downstream Jackson projects until we implement the equivalent method

@JooHyukKim
Copy link
Member

JooHyukKim commented Dec 24, 2024

Is this part of any JSTEP? If not I think we can add somewhere in JSTEP for 2->3 migration doc later on.

@cowtowncoder
Copy link
Member

Is this part of any JSTEP? If not I think we can add somewhere in JSTEP for 2->3 migration doc later on.

Would belong under https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-3 and I think @pjfanning probably noticed my adding #4863 (sort of related). TreeNode additions are bit limited due to Java limitation on co-variance (cannot add return value of Stream<TreeNode> because not legal to override with Stream<JsonNode>) but can do this for property names (since keys are still Strings).

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 24, 2024

@pjfanning Quick question on need/rationale here: I forget details of how Spliterators are useful (as building blocks for Streams... ? ). So would this be useful for actual use cases?

Another question is if this should go in 2.19 instead -- I plan on adding most JsonNode additions, where possible, in 2.19 not just 3.0 (but some will of course need to go there).

@pjfanning
Copy link
Member Author

pjfanning commented Dec 24, 2024

I saw the changes for stream support and spliterators are most used as an iterator that can be split and processed in parallel (eg parallel streams).
The changes don't look very drastic so I'm happy to not merge the changes and wait to see if there is explicit demand for this. What I mean is that changes like this could be added at any time if needed and they don't really need a major release.

@cowtowncoder
Copy link
Member

I saw the changes for stream support and spliterators are most used as an iterator that can be split and processed in parallel (eg parallel streams). The changes don't look very drastic so I'm happy to not merge the changes and wait to see if there is explicit demand for this. What I mean is that changes like this could be added at any time if needed and they don't really need a major release.

Correct, except for one thing: to add method in TreeNode there either needs to be default method implementation, or needs to go in a major version.

I'll have a look at jackson-databind side and see if that makes.

Also, fwtw, I try to keep JSTEP-3 updated with changes to JsonNode / TreeNode, although it's obviously challenging to keep related-but-not-synced things in-sync manually (github not having something for "uber-issues" like... epics or something).

@pjfanning
Copy link
Member Author

I've added a propertyNamesSpliterator default impl.

@cowtowncoder cowtowncoder merged commit 9af646d into FasterXML:master Dec 28, 2024
6 checks passed
@pjfanning pjfanning deleted the spliterator branch December 28, 2024 09:02
@cowtowncoder
Copy link
Member

Realized somewhat late that name needs to be propertyNameSpliterator() since we will use:

  1. Plulars for actual Collection/Set (JsonNode.values())`
  2. But Singulars for streams (valueStream()) and thereby spliterators as well.

Will change accordingly.

cowtowncoder added a commit that referenced this pull request Dec 30, 2024
@JooHyukKim
Copy link
Member

JooHyukKim commented Dec 30, 2024

Realized somewhat late that name needs to be propertyNameSpliterator() since we will use:

Is there a written form of this convention somewhere? We could probably go make one in the main '/Jackson' repo Found in JSTEP

@cowtowncoder
Copy link
Member

Right: there's JSTEP-6 (https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-6) for this. But I need to update that wrt this part of naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants